Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduced to 2 classes: PassiveSocket and SimpleSocket .. and some more fixes/additions #4

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

hayguen
Copy link
Contributor

@hayguen hayguen commented Jun 17, 2016

see source!,
should be easier to use:
PassiveSocket for Server things
and SimpleSocket for everything else

@lethosor
Copy link
Member

Does this maintain source compatibility (i.e. is it possible to still use CActiveSocket)?

@hayguen
Copy link
Contributor Author

hayguen commented Jun 17, 2016

hmmm, you are right, my patch breaks compatibility!
that's why i had to fix/update the examples.
Cause Accept() delivered CActiveSocket*, a child of CSimpleSocket.
I think changing CActiveSocket to a typedef should fix that ...

@hayguen
Copy link
Contributor Author

hayguen commented Jun 17, 2016

in the examples it fixed that problem.
have a look yourself!

@hayguen
Copy link
Contributor Author

hayguen commented Jun 19, 2016

my last commit with new examples and many new aliases (inline functions) and a few bugfixes were merged into this pull request ..

@warmist
Copy link
Member

warmist commented Jun 20, 2016

Have you tried building dfhack with it?

@hayguen
Copy link
Contributor Author

hayguen commented Jun 20, 2016

Nope. I'm using clsocket for extio_rtl_tcp
https://github.com/hayguen/extio_rtl_tcp
and probably for other projects (in future).

There shouldn't be errors building DFHack .. are there?

@warmist
Copy link
Member

warmist commented Jun 20, 2016

oh my bad. Thought that this is not used anywhere else.

@hayguen
Copy link
Contributor Author

hayguen commented Jun 20, 2016

Had found clsockets few years ago. Think, it was at the authors official site, which now is down
It was one of the few or even the only lib with following criteria:

  • Windows and Linux
  • use in commercial software
  • small / easy to use - without full blown framework
  • easy static linking

Had used multicast UDP sockets at that time ...
Now using it again, for TCP reception in the extio_rtl_tcp DLL on Windows.

You see any problem using DFHack/clsocket as "base" for further developments?

@warmist
Copy link
Member

warmist commented Jun 20, 2016

Nope. Just thought that something more popular exists and we're just using this mostly because it's very simple and we started using it.

@hayguen
Copy link
Contributor Author

hayguen commented Jun 20, 2016

There's nothing wrong, when it's simple!

@@ -328,6 +328,10 @@ int32 CPassiveSocket::Send(const uint8 *pBuf, size_t bytesToSend)
case CSimpleSocket::SocketTypeTcp:
CSimpleSocket::Send(pBuf, bytesToSend);
break;
case CSimpleSocket::SocketTypeInvalid:
case CSimpleSocket::SocketTypeTcp6:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Tcp6 be here or part of the previous case for Tcp?

@hayguen
Copy link
Contributor Author

hayguen commented Jul 8, 2016

you are definitely right: that code looks wrong. noticed that also.
in fact i did not change the behaviour, just silenced the warnings from gcc, complaining of missing cases.
before fixing those bug(s), we should have a test program, e.g. extend echoserver and txtoserver for ipv6!

@hayguen
Copy link
Contributor Author

hayguen commented Jul 21, 2016

Found out, that there's much more to do, besides the correct switch case, that IPv6 works correctly:
address resolution is broken/deprecated ..

@lethosor lethosor mentioned this pull request Aug 8, 2016
@lethosor
Copy link
Member

lethosor commented Aug 8, 2016

Do we really need GetNumReceivableBytes()? I had to add checks for _DARWIN in addition to _LINUX to get it to compile, but it's always returning 0. That's probably possible to fix, but I'm still not sure why we need it now if we didn't need it before. The examples only seem to compare it to the number of bytes returned by Receive(), but it doesn't actually affect them.

From here:

Be warned though, the OS doesn't necessarily guarantee how much data it will buffer for you, so if you are waiting for very much data you are going to be better off reading in data as it comes in and storing it in your own buffer until you have everything you need to process something.

And here:

You simply cannot assume that every send() from one end of the connection will result in exactly one recv() on the far end with exactly the same number of bytes.

Basically, if you care about receiving the same number of bytes that were sent, you should probably include the message length in the message, instead of relying on the network to transmit the entire message at once.

I'll still see if I can fix it on OS X if it is needed, but I'm not sure about it.

This is the patch I made to get it to compile, by the way:

diff --git a/src/SimpleSocket.cpp b/src/SimpleSocket.cpp
index 2d0eedd..4c613f9 100644
--- a/src/SimpleSocket.cpp
+++ b/src/SimpleSocket.cpp
@@ -42,7 +42,7 @@
  *----------------------------------------------------------------------------*/
 #include "SimpleSocket.h"

-#ifdef _LINUX
+#if defined(_LINUX) || defined(_DARWIN)
 #include <sys/ioctl.h>
 #endif
 #include <string.h>
@@ -1326,7 +1326,7 @@ int32 CSimpleSocket::GetNumReceivableBytes()
         return -1;

     return (int32)numBytesInSocket;
-#elif defined(_LINUX)
+#elif defined(_LINUX) || defined(_DARWIN)
     int32 numBytesInSocket = 0;
     if ( ioctl(m_socket, FIONREAD, (char*)&numBytesInSocket) < 0 )
         return -1;

@hayguen
Copy link
Contributor Author

hayguen commented Aug 8, 2016

I definitely need it in some other closed-source project, where i replaced the Qt4 Socket stuff.
So it is used and implicitly tested on Windows 32/64 and Linux.
I read the warnings .. but the exact value of available bytes is not necessary.
We could always return a new error for DARWIN, if that's ok?

@lethosor
Copy link
Member

lethosor commented Aug 8, 2016

Just Receive() doesn't work? If you're receiving into a buffer, is it not possible to just use a buffer large enough for whatever you need?

I think I found a way to get it working on OS X, though, if it's necessary.

@hayguen
Copy link
Contributor Author

hayguen commented Aug 8, 2016

The company specific close source has its own abstraction for network/tcp connections, where Qt4 was utilized in the back end. Now, replaced that Qt4 - without touching the application layer / logic :-)
You see, i can't use Receive() :-(

@lethosor
Copy link
Member

lethosor commented Aug 8, 2016

Oh, so you're replacing Qt's equivalent of GetNumReceivableBytes? I guess that would make sense.

@hayguen
Copy link
Contributor Author

hayguen commented Aug 8, 2016

More or less: yes.
Qt3 Compatibility Layer, coming with Qt4, Qt3::QSocketDevice was working fine.
In Qt4, QSocket does handle it's own buffers per thread .. breaking many things!
Now the solution is/was to use clsockets for networking and to allow moving to Qt5.

@hayguen hayguen force-pushed the master branch 2 times, most recently from ce59dda to 9527220 Compare May 3, 2018 13:59
…hread, libstdc++, ..

build examples also on mingw

Signed-off-by: hayati ayguen <[email protected]>
moved Open() and ConnectXX methods from ActiveSocket into SimpleSocket
PassiveSocket now returns SimpleSocket at Accept()
ActiveSocket got unnecessary .. but kept file for compatibility

Signed-off-by: hayati ayguen <[email protected]>
* added example DelayedEchoServer - sends back received data after some delay
* added console/log output to EchoServer example
* added example TxToServer -  sends tcp data to echo server
  utilizes SetSendWindowSize() and fixed Select()/WaitUntilReadable()
* added example UdpServer - prints received data to stderr
* added example TxToUdpServer - sends udp data to udp server

* added alias (inline) CPassiveSocket::Bind()
  for BindMulticast() without multicast group
* fixed BindMulticast() for NULL ptr to mulicast group

* added alias (inline) CSimpleSocket::ConnectTo() for Open()
* added aliases CloseForXX() for Shutdown() variants
* fixed SetReceiveTimeout() and SetSendTimeout() for Windows:
  Windows does not expect/support struct timeval - only milliseconds
* added aliases SetReceiveTimeoutMillis() and SetSendTimeoutMillis()
* added extra options bAwakeWhenReadable and bAwakeWhenWritable for Select(),
  both defaulting to true - as before, but usually not making much sense!
* added aliases WaitUntilReadable() and WaitUntilWritable() with milliseconds
* added aliases GetSocketErrorText() for DescribeError)
* added alias Reveive() with 'char*'
* added alias Send() with  'const char*'
* added aliases Transmit() with 'const uint8*' and 'const char*'
* renamed SetOptionLinger()'s nTime parameter to nTimeInSeconds

Signed-off-by: hayati ayguen <[email protected]>
@hayguen hayguen changed the title reduced to 2 classes: PassiveSocket and SimpleSocket reduced to 2 classes: PassiveSocket and SimpleSocket .. and some more fixes/additions Aug 12, 2020
added GetNumReceivableBytes()
added IsServerSide()
added Get(Peer/Local)(Addr/Port)
allow examples to connect from remote computers
  this is for testing examples (server/client) on different OS/machines
rebase amended bugfix patch from lethosor for DARWIN/OS X

Signed-off-by: hayati ayguen <[email protected]>
* SetConnectTimeoutMillis() was setting the receive timeout
* optimization: CPassiveSocket::Accept() does now waits for result of accept()
  before creating CSimpleSocket()
  - that's interesting for a NonBlocking CPassiveSocket
* added bool CSimpleSocket::IsSocketPeerClosed()
  returning if remote side closed the socket
* made IsBlocking() const
* added inlines for IsSocketPeerOpen(), IsSocketInvalid() and IsNonblocking()
  simplifying logic in if expressions
* added internal PRINT_CLOSE macro for debugging

Signed-off-by: hayati ayguen <[email protected]>
problem scenario: when there's already an error outside the clsocket lib,
  then TranslateSocketError() could pick up the error,
  which was not caused by the clsocket-operation
added ClearSystemError() and use it in some cases
call/use TranslateSocketError() when necessary

Signed-off-by: hayati ayguen <[email protected]>
with kind permission of Procitec:
* bugfixes in multicast UDP operation
* enhancend for multithreaded use:
 - use GetAddrInfo[Static]() instead of deprecated GETHOSTBYNAME()
 - GetIPv4AddrInfoStatic() to allow address resolution without
   a connection instance
* docs and other minor enhancements

Signed-off-by: hayati ayguen <[email protected]>
* mingw is missing inet_pton() function.
 - added replacement implementation
 - switched on with cmake option CLSOCKET_OWN_INET_PTON
* msvc: static build against c runtime with cmake CLSOCKET_SHARED off
* build examples with msvc - except clsocket-example requiring pthread

Signed-off-by: hayati ayguen <[email protected]>
* fixed CSimpleSocket::GetClientPort()
* fixed peer/local address in CPassiveSocket::Accept()
* fixed Linux compilation in CPassiveSocket::Listen()
* added CSimpleSocket::Bind() for tcp/udp client connections
* enhanced all examples: all print local/peer address and port
* fixed output of GetLocalAddr() and GetPeerAddr() in examples
* examples txtoserver and txtoudpserver with optional local binding

Signed-off-by: hayati ayguen <[email protected]>
* looks, some systems return random value!
  directly return 0 in this case

Signed-off-by: hayati ayguen <[email protected]>
@hayguen
Copy link
Contributor Author

hayguen commented Aug 12, 2020

I'm pretty sure #4 (comment) still needs to be addressed for macOS.

yes, you were right. i rebase/amended your patch.
by the way: not having a macOS for testing .. is there a simple way for remote compilation and executing unittests?

I would still like to verify that DFHack works with these changes

of course

might not have much time to do that until next week or so

sure, that's good .. especially after having all this stuff lying around for years

@lethosor
Copy link
Member

I've been meaning to set up GitHub Actions in this repo - I was planning on just Linux (to replace Travis CI) but could probably add macOS and Windows too. That should help make sure it at least compiles on those platforms - not sure how comprehensive tests are currently.

@lethosor lethosor linked an issue Aug 12, 2020 that may be closed by this pull request
@hayguen
Copy link
Contributor Author

hayguen commented Aug 12, 2020

but could probably add macOS and Windows too

sounds great. i've no experience with this github actions kind of stuff ..

not sure how comprehensive tests are currently

(nearly) all test programs are for manual execution .. but we should be able to figure out some automatic tests

* renamed EXPORT to CLSOCKET_API, it's for export and import
* added definition CLSOCKET_NO_EXPORT for not exporting private methods
* cmake: export symbols only for shared library: EXPORT_CLSOCKET_SYMBOLS
* have default visibility hidden - also on linux ..

Signed-off-by: hayati ayguen <[email protected]>
@hayguen
Copy link
Contributor Author

hayguen commented Aug 13, 2020

@lethosor :

and might not have much time to do that until next week or so

i'm not at work for the next 2 weeks from next monday .. but we have several more changes/commits, i'd need to merge ..
you might want to wait until i'm back and get them into the github repo, to test everything at once .. or start testing .. and accept what is already online.
however you prefer ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

function bool CSimpleSocket::Shutdown(CShutdownMode nShutdown)
4 participants